All Hops Encrypted: alpha Kourier support for encrypted for internal traffic #824

nak3 · 2022-04-13T13:02:00Z

As described in #805, service to service traffic does not
support TLS encryption. kourier-internal service does not even open the port.

This patch fixes it.

/cc @skonto @rhuss

knative-prow · 2022-04-13T13:02:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nak3]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nak3 · 2022-04-13T13:04:23Z

@evankanderson knative/serving#11906 did not mention about the internal traffic (*.svc.cluster.local) but our customer requested that "all" traffic so we need to add this. Please let us know if you think we should support it in config-network rather than Kourier specific.

codecov · 2022-04-13T13:05:33Z

Codecov Report

Merging #824 (b532a02) into main (af6ef1e) will decrease coverage by 0.39%.
The diff coverage is 67.85%.

@@            Coverage Diff             @@
##             main     #824      +/-   ##
==========================================
- Coverage   82.22%   81.83%   -0.40%     
==========================================
  Files          18       18              
  Lines        1142     1167      +25     
==========================================
+ Hits          939      955      +16     
- Misses        162      168       +6     
- Partials       41       44       +3

Impacted Files	Coverage Δ
pkg/config/config.go	`0.00% <ø> (ø)`
pkg/generator/caches.go	`71.24% <65.38%> (-1.14%)`	⬇️
pkg/config/configmap.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af6ef1e...b532a02. Read the comment docs.

pkg/config/configmap.go

skonto · 2022-04-14T15:38:02Z

pkg/generator/caches.go

@@ -336,3 +367,12 @@ func newExternalEnvoyListenerWithOneCert(ctx context.Context, manager *httpconnm

 	return envoy.NewHTTPSListener(config.HTTPSPortExternal, []*v3.FilterChain{filterChain}, enableProxyProtocol)
 }
+
+func newInternalEnvoyListenerWithOneCert(ctx context.Context, manager *httpconnmanagerv3.HttpConnectionManager, kubeClient kubeclient.Interface, enableProxyProtocol bool, certSecretName string) (*v3.Listener, error) {


The only diff between internal and external for newXXXEnvoyListenerWithOneCertFilterChain is the namespace and secretaName pair?

os.Getenv(envCertsSecretNamespace) , os.Getenv(envCertsSecretName) vs system.Namespace(), certSecretName,

Should we simplify this by merging newInternalEnvoyListenerWithOneCertFilterChain and newInternalEnvoyListenerWithOneCert to one func and then just the appropriate values for both internal/external cases?

Alternatively, rather than making the cert name configurable, you could hard code it, and use the presence / absence of the cert to activate the encryption on the internal interface.

In my experience, this can work fairly well if the component is installed in its own namespace and isn't expected to share. In that case, you could default the secret name to e.g. kourier-internal-domain-cert or the like, and then document that if that cert is created, Kourier will use it.

Hard cord name is fine, but I assumed that the future plan as:

Create the secret(=certs) automatically by cert-manager or the library in knative.dev/pkg.

Knative always use TLS traffic with the auto generated certs.

At step-2, cluster-cert-secrete in the ConfigMap starts having the default value like:

kind: ConfigMap name: config-kourier cluster-cert-secret: "kourier-internal-domain-cert"

And if users want to use their own certs rather than auto-gen certs, they can set the different value in cluster-cert-secret.

pkg/generator/caches_test.go

skonto · 2022-04-14T16:09:37Z

@nak3 for the coverage reports I dont have access to evaluate what is wrong, obviously some files like config.go could have more coverage but I am no sure. I left a comment in #802

LGTM, minor comments.

evankanderson · 2022-04-14T16:34:28Z

How does this work with Addressable resources? Will they refuse to connect without trusting the (self-signed?) CA for the *.svc.cluster.local address?

I'm concerned about unintended side effects of skipping this into the 1.4 build with only 3-4 business days of testing before the release. If we are going to do it, it should definitely be confined to net-kourier so that side effects can be mitigated by switching specific services to other ingress implementations.

evankanderson

Reading through this, it looks like it doesn't disable HTTP serving, nor does it change the published URL of the KIngress. If that's true (I didn't try running it, and I'm only seeing the changed files, not the final outcome), then I'm less worried about the short-term impact on Addressable.

config/200-config.yaml

pkg/config/configmap.go

config/200-config.yaml

pkg/config/configmap.go

evankanderson · 2022-04-14T16:56:22Z

pkg/generator/caches.go

@@ -336,3 +367,12 @@ func newExternalEnvoyListenerWithOneCert(ctx context.Context, manager *httpconnm

 	return envoy.NewHTTPSListener(config.HTTPSPortExternal, []*v3.FilterChain{filterChain}, enableProxyProtocol)
 }
+
+func newInternalEnvoyListenerWithOneCert(ctx context.Context, manager *httpconnmanagerv3.HttpConnectionManager, kubeClient kubeclient.Interface, enableProxyProtocol bool, certSecretName string) (*v3.Listener, error) {


Alternatively, rather than making the cert name configurable, you could hard code it, and use the presence / absence of the cert to activate the encryption on the internal interface.

In my experience, this can work fairly well if the component is installed in its own namespace and isn't expected to share. In that case, you could default the secret name to e.g. kourier-internal-domain-cert or the like, and then document that if that cert is created, Kourier will use it.

skonto · 2022-04-15T10:27:12Z

@nak3 (ignore my previous comment on cov) the coverage report is here.The files marked with red are the offending ones, need better coverage.
In our codecov file we set up an allowed drop of 1% this PR causes "decrease coverage by 1.23%".
More details about the threshold here. Let's fix it here if possible.

…traffic

nak3 · 2022-04-15T13:33:09Z

The coverage https://codecov.io/gh/knative-sandbox/net-kourier/compare/483cd470dc95d03d63c9c853877d8df104d946e6...4a5366996df81b2524357458d1bb92bc14272dbe# compared with non-related old commits. Just rebased and passed codeconv.

skonto · 2022-04-15T16:42:39Z

/lgtm /hold for @evankanderson

evankanderson

/lgtm
/hold cancel

knative-prow bot requested review from rhuss and skonto April 13, 2022 13:02

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2022

skonto reviewed Apr 14, 2022

View reviewed changes

pkg/config/configmap.go Outdated Show resolved Hide resolved

skonto reviewed Apr 14, 2022

View reviewed changes

pkg/generator/caches_test.go Outdated Show resolved Hide resolved

skonto mentioned this pull request Apr 14, 2022

Fix coverage settings #802

Closed

evankanderson reviewed Apr 14, 2022

View reviewed changes

nak3 added 4 commits April 15, 2022 22:28

All Hops Encrypted: alpha Kourier support for encrypted for internal …

574aa0d

…traffic

Rename configuration name

723a8ea

Fix review comments

ee6baf1

Fix lint

b532a02

skonto added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Apr 15, 2022

evankanderson reviewed Apr 15, 2022

View reviewed changes

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2022

knative-prow bot assigned evankanderson Apr 15, 2022

knative-prow bot merged commit 90ef619 into knative-extensions:main Apr 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All Hops Encrypted: alpha Kourier support for encrypted for internal traffic #824

All Hops Encrypted: alpha Kourier support for encrypted for internal traffic #824

nak3 commented Apr 13, 2022

knative-prow bot commented Apr 13, 2022

nak3 commented Apr 13, 2022

codecov bot commented Apr 13, 2022 •

edited

Loading

skonto Apr 14, 2022 •

edited

Loading

evankanderson Apr 14, 2022

nak3 Apr 15, 2022 •

edited

Loading

skonto commented Apr 14, 2022 •

edited

Loading

evankanderson commented Apr 14, 2022

evankanderson left a comment

evankanderson Apr 14, 2022

skonto commented Apr 15, 2022 •

edited

Loading

nak3 commented Apr 15, 2022

skonto commented Apr 15, 2022

evankanderson left a comment

All Hops Encrypted: alpha Kourier support for encrypted for internal traffic #824

All Hops Encrypted: alpha Kourier support for encrypted for internal traffic #824

Conversation

nak3 commented Apr 13, 2022

knative-prow bot commented Apr 13, 2022

nak3 commented Apr 13, 2022

codecov bot commented Apr 13, 2022 • edited Loading

Codecov Report

skonto Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

evankanderson Apr 14, 2022

Choose a reason for hiding this comment

nak3 Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

skonto commented Apr 14, 2022 • edited Loading

evankanderson commented Apr 14, 2022

evankanderson left a comment

Choose a reason for hiding this comment

evankanderson Apr 14, 2022

Choose a reason for hiding this comment

skonto commented Apr 15, 2022 • edited Loading

nak3 commented Apr 15, 2022

skonto commented Apr 15, 2022

evankanderson left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 13, 2022 •

edited

Loading

skonto Apr 14, 2022 •

edited

Loading

nak3 Apr 15, 2022 •

edited

Loading

skonto commented Apr 14, 2022 •

edited

Loading

skonto commented Apr 15, 2022 •

edited

Loading